Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve snippet-insertion. #941

Merged
merged 7 commits into from
Oct 4, 2023
Merged

Improve snippet-insertion. #941

merged 7 commits into from
Oct 4, 2023

Conversation

L3MON4D3
Copy link
Owner

Right now we handle snippet-insertion (how the snippet is jumped to/from, if history is enabled) pretty naively, by always inserting the snippet inside the currently active node. This leads to annoying issues (jumping back far in the buffer, snippets aren't jumped into when jumping from an buffer-position-adjacent one), whose symptoms could be eliminated with things like region_check_events, but it's time for a proper solution.

This PR fixes these issues by always inserting the snippet at the node which contains the position it is expanded at, and by leaving the currently active snippet if the new snippet is not inside it.

This also enables an easy fix for #859, since we now know exactly which node has to be updated when the text in a nested snippet is changed.
It should also make unlinking snippets due to deleted text more reliable, right now we look for also-affected snippets by following node.next/prev (essentially by simulating jump(1) which is kind of stupid tbh), but if we have a proper tree of expanded snippets, this can be implemented much better (I think :D)

This is by no means finished, but the searching and insertion is implemented, leaving the current snippet and the other fixes enabled by this don't work yet

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch 4 times, most recently from 22d4a1a to 83cacad Compare July 7, 2023 14:43
@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented Jul 7, 2023

Leaving the current node upon insertion should work now, here's a short demo (because I'm enjoying this very much :D)

1688741136.mp4

Notice how the extmark-highlights are dis/enabled when leaving the current node/entering the node the snippet is expanded in, and how the jumps behave as expected

An issue with this new behaviour is that enabling region_check_events is now very annoying, because it will jump to the very end of the buffer. Thinking of just ripping that out, there's no real reason for enabling it anymore once this is merged.

@sandersantema
Copy link

The demo video file seems to be corrupt so I can't see for certain, but I assume this PR is ready for testing? I'll give it a go if so.

@L3MON4D3
Copy link
Owner Author

Oh that would be awesome, thank you!
I'd say wait a bit, there are a few pretty common bugs I should be able to iron out rather quickly, I'll let you know when I think I have the most common ones squashed.
(no idea what's wrong with the video, I'll try uploading it again)

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch from d9f591e to 37a393a Compare July 11, 2023 16:57
@L3MON4D3
Copy link
Owner Author

Alright, got the ones I know about, feel free to take it for a drive :D

@sandersantema
Copy link

I hope this bug reporting isn't too barren, I've got time to try it out but not much time to do in-depth debugging.

This is the first error I've encountered

E5108: Error executing lua ...site/pack/packer/start/LuaSnip/lua/luasnip/util/mark.lua:82: attempt to index a nil value
stack traceback:
        ...site/pack/packer/start/LuaSnip/lua/luasnip/util/mark.lua:82: in function 'pos_begin_end_raw'
        ...ite/pack/packer/start/LuaSnip/lua/luasnip/nodes/util.lua:238: in function 'binarysearch_pos'
        .../pack/packer/start/LuaSnip/lua/luasnip/nodes/snippet.lua:457: in function 'find_snippettree_position'
        .../pack/packer/start/LuaSnip/lua/luasnip/nodes/snippet.lua:602: in function 'trigger_expand'
        ...nvim/site/pack/packer/start/LuaSnip/lua/luasnip/init.lua:228: in function 'snip_expand'
        ...nvim/site/pack/packer/start/LuaSnip/lua/luasnip/init.lua:340: in function 'expand'
        ...hare/nvim/site/pack/packer/opt/nvim-cmp/lua/cmp/core.lua:490: in function <...hare/nvim/site/pack/packer/opt/nvim-cmp/lua/cmp/core.lua:436>
        ...site/pack/packer/opt/nvim-cmp/lua/cmp/utils/feedkeys.lua:47: in function <...site/pack/packer/opt/nvim-cmp/lua/cmp/utils/feedkeys.lua:45>

This happened while autocompleting if ... with the then ... end snippet which afaik comes from lua_ls. This is the relevant cmp mapping:

    ['<Tab>'] = cmp.mapping(function(fallback)
      if luasnip.expandable() and not cmp.get_selected_entry() then
        luasnip.expand()
        cmp.close()
      elseif cmp.visible() then
        cmp.confirm({ select = true })
      else
        fallback()
      end
    end, { 'i', 's' }),

Let me know if you need anymore information.

@L3MON4D3
Copy link
Owner Author

Aaah, I guess this happens if a deleted snippet exists while inserting a new one.. Should be an easy fix 🤞

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch from 89db67a to 56d9997 Compare July 11, 2023 21:23
@L3MON4D3
Copy link
Owner Author

Took a bit too long, but that should be that

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch from 25491aa to 3fa6984 Compare July 12, 2023 06:59
@sandersantema
Copy link

I wouldn't say long but rather very quick 😄. Anyhow I haven't experienced any other bugs. I haven't used it extensively however but will do so in the coming days. If I encounter anything I'll let you know, but so far a very nice improvement!

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch 2 times, most recently from fc13544 to 62c5711 Compare July 12, 2023 20:07
@sandersantema
Copy link

Seems that I might not have actually tested this pr after reporting the first bug, I thought gh pr checkout 941 would do the job but I'm not completely sure if it did or packer might have changed it back to main. So I'm not sure how valid my previous comment was:

Anyhow I haven't experienced any other bugs.

I'm certain I've merged this pr locally now so I'll continue to use it and see if any errors pop up.

@L3MON4D3
Copy link
Owner Author

True, I should've said that it was a bit more than I anticipated :D

I think this is in really good shape now, maybe one or two small errors in the last few commits, but all the big pieces, so to speak, are in place, and should work

Thank you, I'm very happy to finally get rid of that pain-point :)
(I'm currently least confident in gracefully handling deletion of snippets (like deleting their buffer-text), so go wild with that)

@L3MON4D3
Copy link
Owner Author

Seems that I might not have actually tested this pr after reporting the first bug, I thought gh pr checkout 941 would do the job but I'm not completely sure if it did or packer might have changed it back to main. So I'm not sure how valid my previous comment was:

Anyhow I haven't experienced any other bugs.

I'm certain I've merged this pr locally now so I'll continue to use it and see if any errors pop up.

Oh, it must've worked, the traceback contains code only present in this PR

@sandersantema
Copy link

like deleting their buffer-text

What exactly do you mean by that? The text inserted into the buffer after expanding the snippet?

@L3MON4D3
Copy link
Owner Author

Jup, just that.
It causes the marks we use to remember node-positions in the buffer to shift around in weird ways, and we can only really detect this by catching an error, which requires that lots of code just catches these errors and handles them appropriately (by removing the snippet, or passing the error up, so the snippet can be removed somewhere else).
All that is pretty fickle, and I think the code handles the issues I'm aware of correctly, but I haven't yet used it enough to be really confident in it

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch from a086adc to 88043f6 Compare July 14, 2023 21:25
@sandersantema
Copy link

sandersantema commented Jul 15, 2023

Found one :)

The following auto-snippet:

  s(
    {
      trig = ';([^;]*);',
      wordTrig = false,
      regTrig = true,
      trigEngine = 'ecma',
    },
    fmta('_<>', {
      d(
        1,
        function(_, snip)
          return string.len(snip.captures[1]) > 1
              and sn(1, t(string.format('{%s}', snip.captures[1])))
            or sn(1, t(snip.captures[1]))
        end
      ),
    })
  ),

And relevant config:

luasnip.config.setup({
  history = true,
  update_events = 'TextChanged,TextChangedI',
})

Then s;n; expands to _nand s;foo; to _{foo} while on the main branch s;n; expands to s_n and s;foo; to s_{foo} as expected.

@L3MON4D3
Copy link
Owner Author

Ahhhhh it's because we don't yet have a fix for this from main on here xD
Rebasing.. 😅
Had me stumped there for a second :D

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch 2 times, most recently from 12426d5 to 9782d42 Compare July 22, 2023 17:07
@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch from a24ae60 to 8ecab5f Compare July 22, 2023 22:05
@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch from d2c77fa to 1321a35 Compare August 31, 2023 17:44
@leiserfg
Copy link
Contributor

leiserfg commented Sep 1, 2023

s PR is ready for

The video is not corrupted, you are probably using firefox and it does not play the videos @L3MON4D3 uploads.

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch 3 times, most recently from 1697f69 to 214d145 Compare September 23, 2023 18:10
@L3MON4D3
Copy link
Owner Author

A few words on the latest changes (because they don't seem to stop 😅):
history=true would, with this improved model, mean that the (unfortunately) popular jump/tab-combo (ls.jump if possible, tab otherwise) would break horribly (think expanding a snippet somewhere, then a snippet (textually) before that, <Tab> => one would not insert a tab, but jump back to the first snippet).

The obvious solution is to disable history, so the jump from the textually-before snippet is not possible, but that would (currently) also mean that it is not possible to jump back into previous expansions.

So, history is now split into 3 options, one for deleting previous expansions, one for linking these together (ie. allowing jumps between them), and one for linking nested expansions.
Combined with a function for jumping into the node at any position in the buffer (ls.activate_node()), even if the snippet is not linked up, I think there should be enough flexibility to get a (even more I'd say) satisfactory experience, even with the jump/tab-combo.

@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch 6 times, most recently from fe6240e to 8e79ffa Compare September 30, 2023 11:33
@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch 7 times, most recently from 5593d42 to 8c4332b Compare October 3, 2023 19:50
@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch 2 times, most recently from 6f964fb to bb3f44b Compare October 4, 2023 12:59
Before this, upon expanding a new snippet, its jumps were linked up
with the currently active node, no matter its position in the buffer.
The most prominent negative side-effect of this is jumping all over the
buffer if a snippet is not jumped "through" (ie. to its $0).
This finally adresses this properly by inserting snippets into nodes
according to their position in the buffer.
Read the changes to `DOC.md` for more info.

This commit also deprecates `history` in `setup`, prefer the new options
`keep_roots`, `link_roots`, and `link_children`, as they are more
readable. Still, it is very unlikely that compatibility with `history`
will ever be completely removed, so no need to fret about this.
now parents update children, and children parents.
@L3MON4D3 L3MON4D3 force-pushed the better_snippet_insertion branch from 3e5a59b to 2bd74f6 Compare October 4, 2023 13:02
@L3MON4D3 L3MON4D3 merged commit ad089ed into master Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants